CLDR-15954 Add test of unit preferences test #3571

macchiati · 2024-03-16T04:53:48Z

CLDR-15954

The main goal was to add tests of test files, for verification. It grew a bit as I discovered and fixed problems.

common/testData/units/unitLocalePreferencesTest.txt

new test file to check problems in following the spec with locales

common/testData/units/unitPreferencesTest.txt
common/testData/units/unitsTest.txt

New SPDX id
Fixed the pointer to generation program.

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateUnitTestData.java

addition of 'public void generateUnitLocalePreferences() {' to generate the new test data file
and some utility methods
refactoring a bit

tools/cldr-code/src/main/java/org/unicode/cldr/util/GrammarInfo.java

keep the current behavior for which units get grammar.
needed because of a fix in XXX

tools/cldr-code/src/main/java/org/unicode/cldr/util/Rational.java

some general cleanup of Rational, for issues noticed in building test data
Also made Rational inherit from Number.
NaN was not behaving as expected. Reorganized so that the constructor is simple, and the Rational.of() does more checks
That also touches add(), subtract(), ... which are made simpler and more robust
Changed toBigDecimal(), doubleValue() to be cleaner; added floatValue()
Added utilities of(double), intValue(), longValue(), approximatelyEquals(Number)

tools/cldr-code/src/main/java/org/unicode/cldr/util/UnitConverter.java

the code for getSystemsEnum(String unit) wasn't following the comments, thus getting the wrong answer for some compound units.
added a format method, factoring out code that needed to be in various places.

tools/cldr-code/src/main/java/org/unicode/cldr/util/UnitPreferences.java

made the fast map be a safe singleton (well, memoized):
- Supplier<Map<String, Map<String, Multimap<String, UnitPreference>>>> quantityToUsageToRegionToInfo = Suppliers.memoize(() -> getRawFastMap());
fixed the smallest value to be zero, so that the algorithm to get the preferences didn't have to special-case the least item.
moved code from test/generation to here for getting unitPreferences
added API to get all the quantities

tools/cldr-code/src/main/java/org/unicode/cldr/util/Units.java

fixed get short/long to work for either case, because MeasureUnit may or might not have a prefix.

tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestUnits.java

Added Rational tests for NaN, INF
Added testQuantitiesMissingFromPreferences(), to identify when quantities we don't have in preferences
Added test of the unitPreferencesTest.txt, unitsTest.txt, and (new) unitLocalePreferencesTest.txt data files
Also added a testUnitLocalePreferencesTestIcu() so that we could see how ICU was doing without waiting for integration (it is under a flag, so not normally run).
This PR completes the ticket.

ALLOW_MANY_COMMITS=true

See unicode-org#3571

jira-pull-request-webhook · 2024-03-18T22:24:21Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

srl295

still reviewing

srl295 · 2024-03-19T14:11:55Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/GrammarInfo.java

@@ -755,7 +755,11 @@ public static Set<String> getGrammarLocales() {
                    "knot",
                    "dalton",
                    "kilocalorie",
-                    "electronvolt");
+                    "electronvolt",
+                    // The following may be reinstated after 45.


need a TODO ticket link for this?

Filed yesterday: https://unicode-org.atlassian.net/browse/CLDR-17459

good, should ideally be in code

srl295 · 2024-03-19T14:15:50Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/UnitConverter.java

+        int pos = languageTag.indexOf("-u");
+        String localeBase =
+                (pos < 0 ? languageTag : languageTag.substring(0, pos)).replace('-', '_');


better not to propagate locale string slicing

Suggested change

int pos = languageTag.indexOf("-u");

String localeBase =

(pos < 0 ? languageTag : languageTag.substring(0, pos)).replace('-', '_');

final String localeBase = ULocale.forLanguageTag(languageTag).getBaseName();

something like that?

I'm planning on another PR anyway, and will look at this. Since the beta is already tagged, would you mind approving now if there are no substantive issues?

btangmu

rubber-stamp approval; added suggestions for improvement

btangmu · 2024-03-19T17:04:18Z

common/testData/units/unitPreferencesTest.txt

@@ -2,7 +2,7 @@
 # Test data for unit preferences


Add a comment, "DO NOT EDIT THIS FILE, it is autogenerated by GenerateUnitTestData.java"?

btangmu · 2024-03-19T17:05:15Z

common/testData/units/unitsTest.txt

@@ -1,7 +1,7 @@
 # Test data for unit conversions


Add a comment, "DO NOT EDIT THIS FILE, it is autogenerated by GenerateUnitTestData.java"?

btangmu · 2024-03-19T17:06:55Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateUnitTestData.java

+                // This includes the header, so modify the old header if changes are needed!
+                Files.lines(Path.of(CLDRPaths.TEST_DATA + "units/unitLocalePreferencesTest.txt"))
+                        .forEach(line -> formatPwLocale(pwLocale, line, seen));
+                // TODO: add more lines


TODO comment should have a ticket link, ticket shouldn't be closed until comment is removed

btangmu · 2024-03-19T17:10:17Z

tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestUnits.java

+            String expectedQuantity = converter.getQuantityFromUnit(inputUnit, false);
+            assertEquals("Input: Quantity consistency check", expectedQuantity, quantity);
+
+            // TODO handle mixed_unit_identifiers


TODO should have ticket number

I'd like to do that in a subsequent PR. I had to squash, and it is much simpler to do in a follow-on PR.

btangmu · 2024-03-19T17:10:44Z

tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestUnits.java

+            String expectedQuantity = converter.getQuantityFromUnit(sourceUnit, false);
+            assertEquals("Input: Quantity consistency check", expectedQuantity, quantity);
+
+            // TODO check conversion equation (not particularly important


TODO should have ticket and closing parenthesis

macchiati · 2024-03-19T17:25:31Z

I will, however, be waiting until I get the go-ahead from Dragan

srl295

OK for now, comments noted

srl295 · 2024-03-19T17:54:19Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/GrammarInfo.java

@@ -755,7 +755,11 @@ public static Set<String> getGrammarLocales() {
                    "knot",
                    "dalton",
                    "kilocalorie",
-                    "electronvolt");
+                    "electronvolt",
+                    // The following may be reinstated after 45.


good, should ideally be in code

srl295 · 2024-03-19T17:55:10Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/UnitPreferences.java

+            switch (ms) {
+                case "metric":
+                    return "001";
+                case "uksystem":
+                    return "GB";
+                case "ussystem":
+                    return "US";
+                default:
+                    throw new IllegalArgumentException(
+                            "Illegal ms value in: " + locale.toLanguageTag());


should be data driven.. or actually lookup prefernces

srl295 · 2024-03-19T17:55:34Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/UnitPreferences.java

+        throw new IllegalArgumentException("Fast map should always terminate");
+    }
+
+    public String resolveRegion(ULocale locale) {


function should move into supplemental data info

CLDR-15954 Add test of unit preferences test

85ddce1

See unicode-org#3571

macchiati force-pushed the CLDR-15954-add-test-of-unit-preferences-test branch from 5273eca to 85ddce1 Compare March 18, 2024 22:24

macchiati marked this pull request as ready for review March 18, 2024 23:26

macchiati requested review from pedberg-icu and srl295 March 18, 2024 23:26

srl295 reviewed Mar 19, 2024

View reviewed changes

macchiati requested review from srl295 and btangmu March 19, 2024 16:29

btangmu approved these changes Mar 19, 2024

View reviewed changes

srl295 approved these changes Mar 19, 2024

View reviewed changes

macchiati merged commit 0d85978 into unicode-org:main Mar 19, 2024
10 checks passed

macchiati deleted the CLDR-15954-add-test-of-unit-preferences-test branch March 19, 2024 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDR-15954 Add test of unit preferences test #3571

CLDR-15954 Add test of unit preferences test #3571

macchiati commented Mar 16, 2024 •

edited

Loading

jira-pull-request-webhook bot commented Mar 18, 2024

srl295 left a comment

srl295 Mar 19, 2024

macchiati Mar 19, 2024

srl295 Mar 19, 2024

srl295 Mar 19, 2024

macchiati Mar 19, 2024 •

edited

Loading

btangmu left a comment

btangmu Mar 19, 2024

btangmu Mar 19, 2024

btangmu Mar 19, 2024

btangmu Mar 19, 2024

macchiati Mar 19, 2024

btangmu Mar 19, 2024

macchiati commented Mar 19, 2024

srl295 left a comment

srl295 Mar 19, 2024

srl295 Mar 19, 2024

srl295 Mar 19, 2024

CLDR-15954 Add test of unit preferences test #3571

CLDR-15954 Add test of unit preferences test #3571

Conversation

macchiati commented Mar 16, 2024 • edited Loading

jira-pull-request-webhook bot commented Mar 18, 2024

srl295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macchiati Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

btangmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macchiati commented Mar 19, 2024

srl295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macchiati commented Mar 16, 2024 •

edited

Loading

macchiati Mar 19, 2024 •

edited

Loading